Skip to content

fix: dedupe identical same-key markDefs on every block merge - #3195

Merged
christianhg merged 2 commits into
mainfrom
identical-markdefs-non-conflicts
Aug 31, 2026
Merged

fix: dedupe identical same-key markDefs on every block merge#3195
christianhg merged 2 commits into
mainfrom
identical-markdefs-non-conflicts

Conversation

@christianhg

@christianhg christianhg commented Aug 27, 2026

Copy link
Copy Markdown
Member

An annotation's _key is its identity: comments and decorations follow it, and receivers pair patches through it. Two paths treated a byte-identical markDef under the same _key as a conflict and renamed it, churning that identity for content that is the same annotation: insert.block's fragment adjustment (which also duplicated the def through its plain markDefs append), and, forced by it, the collapsed backspace/forward-delete merges. The range-delete merge already deduped (#3187), shipping the asymmetry behind a dedupeEqualMarkDefs flag.

The first commit fixes the root: adjustFragmentKeys compares colliding defs with isDeepEqual; an identical def keeps its key, is excluded from the appended defs, and its spans' marks resolve to the destination's copy. Different-content defs rename as before, pinned by a contrast test that fails when the equality branch is forced. The second commit deletes the flag: the planner always keeps an identical def unrenamed and inside the merging block (it must stay there, since insert.block parses blocks standalone and strips marks that don't resolve in the block's own markDefs), reporting it so mergeBlock skips the append.

The user-visible delta beyond the rename churn: splitting an annotated block and merging it back now rejoins the annotation into one def under its original key. The gherkin scenario that pinned the old duplicate-def outcome under a "possible wrong behaviour" warning now pins the rejoin. Undo of a deduped merge restores both original blocks byte-identically, pinned by an undo-redo-undo round-trip; wire-catalogue fixtures are untouched.


Note

Medium Risk
Changes core block-merge and insert.fragment key collision logic for annotations, which affects document shape, patches, and undo; behavior is well-tested but any consumer assuming renamed keys on identical defs could differ.

Overview
When two blocks share a markDef with the same _key and the same content, the editor no longer treats that as a key conflict and mints a new annotation id. Block merges (backspace/delete at boundaries, range-delete merges) and text fragment inserts (insert.block / paste-into-block) now use deep equality: identical defs keep their _key, spans keep pointing at them, and duplicate defs are not appended.

planMergeKeyRenames always dedupes equal collisions and returns dedupedMarkDefKeys so merge callers skip re-adding those defs; the old dedupeEqualMarkDefs toggle is removed. adjustFragmentKeys applies the same rule for fragment insertion. Different defs under the same key still get renamed as before.

User-visible: split an annotated line and merge it back → one link under the original key (gherkin updated). Comments, decorations, and patch consumers that track annotation _key should see less spurious renames.

Reviewed by Cursor Bugbot for commit fd390fc. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented Aug 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
portable-text-editor-documentation Ready Ready Preview Aug 31, 2026 9:05am
portable-text-example-basic Ready Ready Preview Aug 31, 2026 9:05am
portable-text-playground Ready Ready Preview Aug 31, 2026 9:05am

Request Review

@changeset-bot

changeset-bot Bot commented Aug 27, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fd390fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@portabletext/editor Patch
@portabletext/plugin-character-pair-decorator Patch
@portabletext/plugin-dnd Patch
@portabletext/plugin-emoji-picker Patch
@portabletext/plugin-input-rule Patch
@portabletext/plugin-list-index Patch
@portabletext/plugin-markdown-shortcuts Patch
@portabletext/plugin-one-line Patch
@portabletext/plugin-paste-link Patch
@portabletext/plugin-sdk-value Patch
@portabletext/plugin-table Patch
@portabletext/plugin-typeahead-picker Patch
@portabletext/plugin-typography Patch
@portabletext/toolbar Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Bundle Stats

✅ No significant changes.

All scenario measurements (7)

🗺️ @portabletext/editor / @portabletext/editor · @portabletext/editor / @portabletext/editor/behaviors · @portabletext/editor / @portabletext/editor/plugins · @portabletext/editor / @portabletext/editor/selectors · @portabletext/editor / @portabletext/editor/traversal · @portabletext/editor / @portabletext/editor/utils · @portabletext/markdown / @portabletext/markdown · Artifacts

Scenario Kind Bundle (raw / gzip) Gzip change Import time Import change
⚪ @portabletext/editor / @portabletext/editor export 1.09 MB / 253.7 KB +356 B, +0.1% 73 ms +3 ms, +4.7%
⚪ @portabletext/editor / @portabletext/editor/behaviors export 4.0 KB / 1.4 KB None 2 ms -0 ms, -2.1%
⚪ @portabletext/editor / @portabletext/editor/plugins export 5.1 KB / 1.8 KB None 7 ms +0 ms, +2.5%
⚪ @portabletext/editor / @portabletext/editor/selectors export 93.7 KB / 21.3 KB None 8 ms +0 ms, +0.5%
⚪ @portabletext/editor / @portabletext/editor/traversal export 41.7 KB / 10.8 KB None 6 ms +0 ms, +1.1%
⚪ @portabletext/editor / @portabletext/editor/utils export 33.1 KB / 8.7 KB None 6 ms +0 ms, +4.0%
⚪ @portabletext/markdown / @portabletext/markdown export 272.2 KB / 79.6 KB None 41 ms -1 ms, -1.5%

Significant means at least 1.0 KB and 1% gzip, or at least 5 ms and 10% import time.

@christianhg
christianhg marked this pull request as ready for review August 31, 2026 08:22
@christianhg
christianhg force-pushed the identical-markdefs-non-conflicts branch from cad0f5e to 4069dfe Compare August 31, 2026 08:25
@christianhg
christianhg force-pushed the identical-markdefs-non-conflicts branch from 4069dfe to 643b9ab Compare August 31, 2026 08:28
@christianhg
christianhg force-pushed the identical-markdefs-non-conflicts branch from 643b9ab to 638cde5 Compare August 31, 2026 08:53
…g text block fragments

`adjustFragmentKeys` renamed a fragment markDef whenever its `_key`
existed on the destination block, on key membership alone. A def that
is byte-identical to the destination's is the same annotation, so the
rename churned identity for nothing: consumers that track annotations
by `_key` (comments, decorations) lost the annotation they were
following, and the caller's plain markDefs append then stored the same
content twice under two keys.

Colliding defs are now compared with `isDeepEqual`: an identical def
keeps its key, is excluded from the defs appended into the
destination, and its spans' `marks` stay untouched, so they resolve
to the destination's copy. Same-key defs with different content rename
exactly as before, pinned by a contrast test that fails when the
equality branch is forced.

One rider: `isDeepEqual` gains an explicit `boolean` return type.
The inferred type predicate narrowed the non-equal branch of same-type
comparisons to `never`, which broke compilation at the new call
site.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7fea0d4. Configure here.

Comment thread packages/editor/tests/block-merge-duplicate-keys.test.tsx Outdated
`planMergeKeyRenames` carried a required `dedupeEqualMarkDefs`
flag: the range-delete merge deduped identical same-key defs, but the
collapsed backspace/forward-delete merges had to keep renaming them,
because their `renamedBlock` feeds `insert.block`, where
`parseBlock` parses the block standalone and strips any mark that
does not resolve in the block's own `markDefs`: a deduped def's
spans would have lost the annotation entirely.

With `insert.block`'s fragment adjustment now treating identical
defs as non-conflicts, that constraint is gone and the flag deletes.
The planner always keeps an identical same-key def unrenamed and
inside `renamedBlock.markDefs` (the block must carry the def so
`parseBlock` resolves its marks) and reports it in
`dedupedMarkDefKeys`; `mergeBlock` skips appending those keys.

Net behavior change: the collapsed merges now dedupe. Splitting an
annotated block and merging it back rejoins the annotation into one
def under its original key, where it previously duplicated the def and
re-keyed half the spans; the `annotations-across-blocks` gherkin
scenario that pinned the old outcome under a 'possible wrong
behaviour' warning now pins the rejoin, and the `unique-sibling-keys`
pin of the same shape follows. Undo of a deduped merge restores both
original blocks byte-identically, pinned by an undo-redo-undo
round-trip. The range-delete path's behavior is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant